fix(fs-router) check for routes with / first, then without #2258
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2258 +/- ##
=======================================
Coverage 93.77% 93.77%
=======================================
Files 65 65
Lines 1639 1639
Branches 368 368
=======================================
Hits 1537 1537
Misses 67 67
Partials 35 35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…ath-is-never-found-in-Java-view-registry
packages/java/endpoint/src/main/java/com/vaadin/hilla/route/ClientRouteRegistry.java
Outdated
Show resolved
Hide resolved
packages/java/endpoint/src/main/java/com/vaadin/hilla/route/ClientRouteRegistry.java
Outdated
Show resolved
Hide resolved
| } | ||
| return null; | ||
| }).filter(Objects::nonNull).findFirst().orElse(null); | ||
| } |
There was a problem hiding this comment.
Just bypassing here: Did you consider security here? I remember some CVEs in the recent past which originated from security confusion based on trailing slash..
Additionally there is this recent Spring Framework Change to keep in mind: spring-projects/spring-framework#28552
There was a problem hiding this comment.
This code doesn't actually change which content is rendered. Its purpose is to try to give the same view permission response that you would get on the client where the navigation is handled by the React Routed.
By doing some manual testing, I've seen that when two routes are present which are only different on the trailing slash, React Router always renders the one with the slash (e.g. you cannot see /about if /about/ exists). But it is also forgiving: /about/ will show the view associated to /about if an exact match is not there.
We don't have this information here, since the new File Router only stores the path items (e.g. ["about"]). Also, the root path is stored as "", but it must match "/".
This PR modifies the code that tries to find the route that React Router would render given a path. In that sense, it should not open any additional security breach.
…ath-is-never-found-in-Java-view-registry
…ath-is-never-found-in-Java-view-registry
…ath-is-never-found-in-Java-view-registry
…ath-is-never-found-in-Java-view-registry
|
cromoteca
deleted the
fix/2244/file-router-the-view-for-the-root-path-is-never-found-in-Java-view-registry
branch
When searching for the current view in Java, the trailing slash can cause misses.
This PR makes sure that both cases are handled, giving priority to the version with trailing slash, because that's what React Router does.
Fixes #2244